Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: prevent error when forwarding user attributes to kits while kit … #947

Merged
merged 7 commits into from
Dec 17, 2024
Merged

fix: prevent error when forwarding user attributes to kits while kit … #947

merged 7 commits into from
Dec 17, 2024

Conversation

mmustafa-tse
Copy link
Contributor

…blocking enabled and unplanned UAs are allowed

Summary

  • A customer provided that an error is thrown when enabling kit blocking or user attributes in the UI and having unplanned user attributes to be allowed, this result in a conflict where dataPointMatchLookUps for user_attributes will be null since and thus when we try to look into a key of null, the error is thrown.

Testing Plan

  • [Y] Was this tested locally? If not, explain why.
  • Unit tests did not fail and E2E tested

Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)

alexs-mparticle
alexs-mparticle requested changes Nov 8, 2024
View reviewed changes
src/kitBlocking.ts Outdated
}
if (!matchedAttributes[key]) {
return true
var matchedAttributes = this.dataPlanMatchLookups['user_attributes'];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var matchedAttributes = this.dataPlanMatchLookups['user_attributes'];
const matchedAttributes = this.dataPlanMatchLookups['user_attributes'];

src/kitBlocking.ts Outdated
return false
}

if (this.blockUserAttributes && typeof matchedAttributes === "object") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is checking this.blockUserAttributes necessary? We're already checking for it's existence a few lines earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually thinking about that and it did seem redundant

rmi22186
rmi22186 reviewed Nov 14, 2024
View reviewed changes
Copy link
Member

@rmi22186 rmi22186 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a test added to this that fails with the previous code, but passes with the current code.

@mmustafa-tse mmustafa-tse force-pushed the fix/dataPlanUserAttributeBlocking branch from 964fb18 to 1e5077a Compare December 3, 2024 20:14
@mmustafa-tse
Copy link
Contributor Author

@rmi22186 @alexs-mparticle - added unit test and confirmed it was passing with my code change in KitBlocking.ts and was failing when testing the old code prior to my changes

rmi22186
rmi22186 reviewed Dec 4, 2024
View reviewed changes
test/src/tests-kit-blocking.ts Outdated
Comment on lines 629 to 630
expect(() => { kitBlocker.isAttributeKeyBlocked('unplannedAttr') }).to.not.throw(TypeError, /Cannot read properties of undefined \(reading 'unplannedAttr'\)/)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add another expect for kitBlocker.isAttributeKeyBlocked('unplannedAttr' to equal true)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the case and confirmed this should be expected to be false

rmi22186
rmi22186 requested changes Dec 4, 2024
View reviewed changes
Copy link
Member

@rmi22186 rmi22186 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments, otherwise lgtm!

@mmustafa-tse mmustafa-tse requested a review from rmi22186 December 4, 2024 19:53
rmi22186
rmi22186 reviewed Dec 4, 2024
View reviewed changes
src/kitBlocking.ts Outdated Show resolved Hide resolved
@mmustafa-tse mmustafa-tse requested a review from rmi22186 December 4, 2024 20:19
@rmi22186 rmi22186 force-pushed the fix/dataPlanUserAttributeBlocking branch from 1879acd to f686ce7 Compare December 17, 2024 20:29
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@rmi22186 rmi22186 merged commit fdbc6ba into mParticle:development Dec 17, 2024
20 of 21 checks passed
github-actions bot pushed a commit that referenced this pull request Jan 6, 2025
@mparticle-automation
chore(release): 2.31.1 [skip ci]
62c7707 
## [2.31.1](v2.31.0...v2.31.1) (2025-01-06)

### Bug Fixes

* Check for uploader to prevent errors when calling upload() too early ([#938](#938)) ([4342b0a](4342b0a))
* Forward user attributes to kits properly when kit blocking is enabled and unplanned UAs are allowed ([#947](#947)) ([fdbc6ba](fdbc6ba))
@mparticle-automation
Copy link
Collaborator

🎉 This PR is included in version 2.31.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexs-mparticle alexs-mparticle alexs-mparticle approved these changes

@rmi22186 rmi22186 rmi22186 approved these changes

Assignees
No one assigned
Labels
released wait to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mmustafa-tse @mparticle-automation @rmi22186 @alexs-mparticle